Repository navigation
Conversation
31629c6 to
68cc859
Compare
|
Cargo stabilized @rfcbot merge compiler |
|
@Urgau has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
cc @Walnut356 (since you may be interested as well) |
Can someone explain more about this dependency relation? |
|
The change creates a compatibility guarantee for toolchain's debugger support for a format defined by Cargo. "Inverting the dependency relation" may be a too strong word, though. Just now debugger and build tool is coupled to some extent, though for the majority it has net benefit. |
|
It looks like if the file doesn't exist or is not in the format we expect, we'll print a warning message but the debugging experience (loading of visualizers, etc) is otherwise not degraded by the failure. Is that correct? |
This is a silent skip, as trim-paths is off by default for now in Cargo. Changing the default is tracked in rust-lang/cargo#17586.
Yes. Correct. |
## Stabilization report
### What is stabilized
With trim-paths `object` scope and debuginfo on,
Cargo writes `<artifact>.trim-paths.json` beside each final artifact.
The "unremap" file maps the remapped paths back to local ones.
This stabilizes debugger script changes:
`rust-gdb` and `rust-lldb` by default load `{gdb,lldb}_trim_paths.py`,
which read the unremap files and apply them
through GDB `substitute-path` or LLDB `target.source-map`
to set up source file path substitutions.
### What changes
* After stabilization, set `RUST_{GDB,LLDB}_TRIM_PATHS=0` to opt out.
Before, this needed `RUST_{GDB,LLDB}_TRIM_PATHS=unstable`.
* After stabilization, both scripts are shipped on all channels.
Before, they were shipped only on nightly.
### Why we don't want to stabilize
* This makes debugger scripts relying on Cargo-specific file format,
inversing the dependency relation.
* There is no integration test for them.
(and debugger scripts actually don't have many tests)
### Docs
See Cargo's the "Unremap files" section:
<https://github.com/rust-lang/cargo/blob/ac74ae3c603b9efce95b07383b46d3103e834d9a/doc/book/src/reference/profiles.md?plain=1#L337-L386>
68cc859 to
7e1b19b
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Did we look at what other ecosystems are doing in this space? I'm loosely aware that Javascript has sourcemaps for a similar-ish goal -- though I think those are commonly embedded in the file itself as a comment? With regards to the location we look for the file, is the relative location the right approach? How does that interact with (IIUC) Cargo's goal to not uplift artifacts that aren't intended for user consumption? Is there some way for a user to more explicitly specify the path (e.g., Maybe the right way to summarize all of that is that it's not entirely clear to me the scope of this stabilization: is this just stabilizing that our debugger scripts will load such a file, but not guaranteeing much else? E.g., where / who owns FCP on the file format and documentation for it (did Cargo already do that)? |
There was a problem hiding this comment.
In reply to @Mark-Simulacrum's #163910 (comment):
Did we look at what other ecosystems are doing in this space? I'm loosely aware that Javascript has sourcemaps for a similar-ish goal -- though I think those are commonly embedded in the file itself as a comment?
We looked at remap part, but not too much in the unremap part. Some goals here are binary reproducibility and local developer experience so embedding any absolute paths is not a "base" solution here. Could be extensions of course.
The other thing to compare is go's -trimpath flag. Go is a bit different as every module has a clear identity even for local ones, though go doesn't have "unremap" concept, and mostly relies on their debugger to guess and requires kinda the exact checkout layout for finding sources.
As for JavaScript sourcemaps URL, if they don't contain absolute maps across builds and is reproducible enough we could definitely extend them in the unremap files, or embed them in the object I think.
With regards to the location we look for the file, is the relative location the right approach? How does that interact with (IIUC) Cargo's goal to not uplift artifacts that aren't intended for user consumption?
Cargo only put trim-paths aside final binaries, like dSYM or PDB files. This doesn't seem to be an issue.
Is there some way for a user to more explicitly specify the path (e.g.,
RUST_{GDB,LLDB}_TRIM_PATHS=/path/to/file.json), and/or should there be?
Not sure if we need to, but users can set their own substitution rules if needed. Doesn't need to tie to the unremap files.
Does our gdb/lldb integration allow for fetching the file from debuginfod servers (does that make sense conceptually)?
Not yet I believe, see this and this. And there is one real gap of the current remap rules: the local workspace source is mapped to path relative to workspace root (DW_AT_comp_dir = "."). The reason of this decision is a tradeoff of local debug experience. With relative paths users can use debugger without any substitution rules for their local packages.
Related to the debuginfod aspect, do we expect any compatibility and/or performance issues from using JSON here? How big are these files?
Small enough (for cargo itself is 462 bytes). JSON is parsed once per object file or module load.
As aforementioned the current remap rules may not be useful for debuginfod. Need some pre/post-process. And anyway debuginfod is not supported at this moment in rustc and related tools. However, a future extension is that Cargo allows extending and customizing the remap rules, which we kinda have one for bootstrap for remapping to /rustc/<sha>.
Maybe the right way to summarize all of that is that it's not entirely clear to me the scope of this stabilization: is this just stabilizing that our debugger scripts will load such a file, but not guaranteeing much else? E.g., where / who owns FCP on the file format and documentation for it (did Cargo already do that)?
True, debugger scripts load them, though the v1 schema is stabilized but exact remap prefixes is subject to change, and adding fields later doesn't break the compatibility per Cargo JSON policy (that one is more for cargo metadata but it is in general our attitude towards JSON outputs). And yes Cargo did an FCP. See the doc and the links in the updated PR description.
There was a problem hiding this comment.
Thanks! I think that answers my questions enough that I'm comfortable checking a box (done). It seems plausible that we'll want/need a v2 format (or extra fields in v1) for some of the goals based on it, if I'm understanding right, but that doesn't seem like a blocker for this stabilization.
Stabilization report
What is stabilized
With trim-paths
objectscope and debuginfo on, Cargo writes<artifact>.trim-paths.json[^1] beside each final artifact. This "unremap" file, maps the remapped paths back to local ones. The file format and remapping rules are stabilized as part of FCP on Cargo side: rust-lang/cargo#17488This stabilizes debugger script changes:
rust-gdbandrust-lldbby default load{gdb,lldb}_trim_paths.py, which read the unremap files and apply them through GDBsubstitute-pathor LLDBtarget.source-mapto set up source file path substitutions.Note that we already apply an unremap for
/rustc/$RUSTC_COMMIT_HASHpaths, so it wouldn't be a new capability per se, just an extension of it.What changes
RUST_{GDB,LLDB}_TRIM_PATHS=0to opt out.Before, it required
RUST_{GDB,LLDB}_TRIM_PATHS=unstableto enable.Before, they were shipped only on nightly.
Why we don't want to stabilize
(and debugger scripts actually don't have many tests)
Docs
See Cargo's the "Unremap files" section: https://doc.rust-lang.org/nightly/cargo/reference/profiles.html#unremap-files.
Should be shipped after #163881 (or along with that)